Skip to content

docs(reanalyze): add DCE refactor plan #8043

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

docs(reanalyze): add DCE refactor plan #8043

wants to merge 3 commits into from
+551 −122

Conversation

@cristianoc
Copy link
Collaborator

@cristianoc cristianoc commented Nov 29, 2025

This PR introduces a planning document for refactoring the reanalyze dead code analysis into a pure, order-independent pipeline.

  • Adds , which:
    • Describes the desired end-state architecture for dead code analysis.
    • Catalogues current sources of mutation and order dependence.
    • Breaks the refactor into small, behaviour-preserving steps that can be tackled in independent PRs.

There are no behavioural changes in this PR: it only adds documentation to coordinate subsequent work on the dead code analysis pipeline.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 29, 2025 •
edited
Loading

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@8043

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@8043

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@8043

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@8043

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@8043

@rescript/runtime

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/runtime@8043

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@8043

commit: ae08f4b

@fhammerschmidt
Copy link
Member

fhammerschmidt commented Nov 29, 2025

I don't see an explicit mention of monorepos or npm/yarn/pnpm workspaces. In this context "dependencies" only refer to other files, but there is still the missing use case when one has shared code like this:

.
├── common
│   ├── package.json
│   ├── rescript.json
│   └── src/
├── mobile
│   ├── package.json
│   ├── rescript.json
│   └── src/
├── web
│   ├── package.json
│   ├── rescript.json
│   └── src/
├── package.json
└── rescript.json

where the root package.json contains a workspace field like so:

"workspaces": [
  "common",
  "mobile",
  "web"
]

and the root rescript.json has

"dependencies": [
  "common",
  "mobile",
  "web",
]

And common/rescript.json is independent of any internal package (may still have external dependencies in a real-world setup):

"dependencies": []

Whereas both web/rescript.json and mobile/rescript.json are dependent on the common package and thus contain:

"dependencies": [
  "common"
]

And we want DCE to mark stuff as dead that is part of common but not used anywhere in the mobile or web package.

This needs to be an option to configure because when e.g. developing a library with tests, I might have tests dependent on the main package, but not 100 % coverage.

The knowledge about package manager monorepos is mostly in rewatch. I imagine if this is all pure and separated now the final result can be recalculated based on a reanalyze includeDependencies: ["common"] option or something.

Also tagging @nojaf and @DZakh who are users of bun and pnpm monorepos.

@cristianoc
Copy link
Collaborator Author

cristianoc commented Nov 29, 2025

I don't see an explicit mention of monorepos or npm/yarn/pnpm workspaces. In this context "dependencies" only refer to other files, but there is still the missing use case when one has shared code like this:


.

├── common

│   ├── package.json

│   ├── rescript.json

│   └── src/

├── mobile

│   ├── package.json

│   ├── rescript.json

│   └── src/

├── web

│   ├── package.json

│   ├── rescript.json

│   └── src/

├── package.json

└── rescript.json

where the root package.json contains a workspace field like so:

"workspaces": [

  "common",

  "mobile",

  "web"

]

and the root rescript.json has

"dependencies": [

  "common",

  "mobile",

  "web",

]

And common/rescript.json is independent of any internal package (may still have external dependencies in a real-world setup):

"dependencies": []

Whereas both web/rescript.json and mobile/rescript.json are dependent on the common package and thus contain:

"dependencies": [

  "common"

]

And we want DCE to mark stuff as dead that is part of common but not used anywhere in the mobile or web package.

This needs to be an option to configure because when e.g. developing a library with tests, I might have tests dependent on the main package, but not 100 % coverage.

The knowledge about package manager monorepos is mostly in rewatch. I imagine if this is all pure and separated now the final result can be recalculated based on a reanalyze includeDependencies: ["common"] option or something.

Also tagging @nojaf and @DZakh who are users of bun and pnpm monorepos.

This is a behavior preserving refactor.
Which btw makes it easier to add features in future.

@cristianoc
refactor(reanalyze): thread binding state and remove globals
687ce52
@cristianoc
reanalyze: localise dead-code binding and reporting state
ae08f4b 
- Introduce BindingContext in DeadValue to track the current binding location during dead-code traversal, so binding context is explicit and locally encapsulated.
- Introduce ReportingContext in DeadCommon to track, per file, the end position of the last reported value when deciding whether to suppress nested warnings.
- Replace addValueReference_state with addValueReference ~binding, so value-reference bookkeeping is driven by an explicit binding location rather than a threaded analysis state.
- Update dead-code value and exception handling to use the new addValueReference API.
- Refresh DEADCODE_REFACTOR_PLAN.md to mark these state-localisation steps as completed and to narrow the remaining follow-up to making the binding context fully pure.
- Verified with make test-analysis that behaviour and expected outputs remain unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cknitt cknitt Awaiting requested review from cknitt

@fhammerschmidt fhammerschmidt Awaiting requested review from fhammerschmidt

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cristianoc @fhammerschmidt